Add support for TVirtualMCSensitiveDetector. #889

kresan · 2019-04-25T11:34:48Z

Inherit FairModule from TVirtualMCSensitiveDetector, which allows to use VMC stepping.
Tested with Tutorial1.
Requires changes in detector interface.

sawenzel · 2019-04-29T14:10:41Z

Could you please summarize shortly what is the idea behind this change? Will it be backward compatible/optional or will it lead to deep changes?

kresan · 2019-05-06T11:51:07Z

It is still work in progress. Idea is to simplify handling of sensitive detectors in FairRoot. Currently foreseen as a default solution. Would require changes in FairDetector interface, which are not backward compatible. Changes are in ProcessHits method.

kresan · 2021-02-09T08:42:00Z

@MohammadAlTurany , Looking into it again, I have realised that the experiments will need to change ProcessHits method in every detector class

karabowi · 2021-02-09T08:50:59Z

can you shortly describe, what the changes would have to be?

kresan · 2021-02-09T10:05:12Z

The declaration:

virtual Bool_t ProcessHits(FairVolume* v = 0) = 0;

will be changed to:

virtual void ProcessHits()=0;

karabowi · 2021-02-09T10:14:53Z

Looking at the code, you've also moved the function from FairDetector to FairModule.
What is the reason to remove its argument and return value?
Can't it stay it used to be?

MohammadAlTurany · 2021-02-09T10:16:06Z

This change is needed only if you went to use the sensitive detector or did I miss something here?

fuhlig1 · 2021-02-09T10:16:59Z

I thought there was a wrapper in FairRoot which calls the old function if the new one isn't available.

kresan · 2021-02-09T10:36:41Z

@karabowi We used to take care about bookkeeping of sensitive volumes in FairRoot. Idea of this PR is to let VMC do it. The new function is VMC requirement.
@MohammadAlTurany I thought we want to get read of the parts where we have to handle sensitive volumes on our own completely. Does it make sense to keep both options?
I agree to Florian that a wrapper can help here.
But, we have to be aware, that FairVolume parameter of the overloaded methods might have been used in the experiment code. How do we handle this with the wrapper?

MohammadAlTurany · 2021-02-09T11:23:36Z

One could get the current volume and the corresponding FairVolume and call the old method, at least for some time to allow the experiments to move smoothly to the new one!

karabowi · 2021-02-09T14:33:48Z

With the current state of this branch, there seems to be no any MC points stored in the output files.

kresan · 2021-02-10T02:16:56Z

@karabowi thanks for testing. Probably something went wrong while rebasing. The whole thing will be revisited completely. I will open another PR soon.

fuhlig1 · 2022-06-01T08:45:29Z

I started testing the Pull Request for CbmRoot and had several problems which are spread over the whole application stack. For my tests I used FairSoft apr21p2 and FairRoot v18.6.7 as baseline since these are the default versions used by CbmRoot. To compare to the PR I cherry-picked the changes from the PR into v18.6.7.

Compilation of the patches FairRoot as well as CbmRoot worked but when running out transport tests all of them failed with an assertion in the Initialize() function of one of our detector classes. The assertion checked that the function was only called once but obviously it was called twice.
When deriving FairModule from TVirtualMCSensitiveDetector the Initialize() function of Detectors/Modules is called from VMC and we need to remove calling the function from out FairMCApplication.
After the change in FairRoot and several fixes in CbmRoot I was able to run the tests without errors. When comparing the results produced with the patched and unpatched versions they were identical for most of our detector systems but for two of them only part of the MCPoints were produced.
After two days of debugging I probably found the problem in TGeant3. If I am correct the issue is already fixed with v4-0 which is the default for FairSoft apr22. I am currently compiling the new FairSoft version to confirm that with this new version the problem is fixed.

Preliminary comparison of execution times of the patched and unpatched FairRoot versions don't show any observable difference.

ChristianTackeGSI · 2022-06-02T08:25:15Z

@fuhlig1
Could you add this double-calling-prevention assert to some of the FairRoot tests/examples please? Maybe open it in a new PR, so that we get this test before we merge the TVirtualMCSensitiveDetector PR?

fuhlig1 · 2022-06-07T09:56:49Z

@ChristianTackeGSI,

the requested test case is described in #1195 and implemented with #1196.

fuhlig1 · 2022-06-07T13:26:54Z

@kresan,

sorry but I think I messed up the PR a minute ago. There was a conflict which I tried to solve using the web editor but it seems to me that instead off doing a rebase the webpage did a merge. Could you please have a look.

kresan · 2022-06-08T07:11:49Z

@fuhlig1, I reverted back your merge action. Now we need to resolve conflicts properly.

fuhlig1 · 2022-06-08T07:13:39Z

@kresan,

thanks. Could you solve the conflicts. I fear if I do it from the web frontend we run into the same problem.

ChristianTackeGSI · 2022-06-20T14:46:04Z

We noticed in this morning's meeting, that we should consider raising the minimum required geant3 version to the one containing some fixes relevant to this pull request.

If we do so, it should be done in the CMakeLists.txt.

ChristianTackeGSI · 2022-06-20T14:47:43Z

Also before we merge this one, we probably should do a rebase/cleanup of the commits.

And before we merge it, we should get clear on our release planning (this currently targets 19.2, not 19.0).

Remove the double function definition.

Define the void FairDetector::ProcessHits() function, which calls the depracated Bool_t ProcessHits(FairVolume*) of derived classes. Update the FairMCApplication to match the update of the VMC: introduction of EndOfEvent function called before the TVirtualMCSensitiveDetectors->EndOfEvent(). Implement the temporary FairMCApplication::GetFairVolume() function to keep the backward compability. Cleanup of FairModule, FairMCApplication double code.

FairRoot allows for construction of clones volumes using the volume_name#{clone_number} scheme. Changing FairMCApplication::ConstructSensitiveDetectors() to support this.

Since VMC is initializing TVirtualMCSensitiveDetector, removed detector->Initialize() from FairMCApplication. Removed the GetFairVolume function used by FairDetector::ProcessHits(). It now calls the deprecated function with NULL argument. Changed the propagator example to use the new ProcessHits().

ChristianTackeGSI · 2023-01-30T19:41:20Z

base/sim/FairMCApplication.cxx

+            std::map<std::string, FairModule*>::iterator it;
+            it = cloneVolumeMap.find(volName);


Suggested change

std::map<std::string, FairModule*>::iterator it;

it = cloneVolumeMap.find(volName);

auto it = cloneVolumeMap.find(volName);

ChristianTackeGSI · 2023-01-30T19:43:22Z

base/sim/FairMCApplication.cxx

+        Int_t id = fMC->CurrentVolID(copyNo);
        id = fMC->CurrentVolID(copyNo);


You're assigning twice the same?

Suggested change

Int_t id = fMC->CurrentVolID(copyNo);

id = fMC->CurrentVolID(copyNo);

Int_t id = fMC->CurrentVolID(copyNo);

ChristianTackeGSI

I have added a few minor comments.

In reality, I would like to discuss the whole design with people interested. Because I think, it's making things more and more complex and unmaintainable in the long run. It took me a few iterations to start to understand why things are done this way. And I have some ideas / questions about this.

base/sim/FairMCApplication.cxx

base/sim/FairMCApplication.h

base/sim/FairModule.h

examples/advanced/propagator/src/FairTutPropDet.cxx

Removed FairDetector::DefineSensitiveVolumes() function, the functionality moved to FairMCApplication private special copy constructor. Removed detector initialization from FairMCApplication::RegisterOutput(), the detectors are initialized in the VMC. Added few overrides for ProcessHits.

Removed the IsSensitive() function. Increase the number of events to run in the MT mode to 20.

kresan added the in progress label Apr 25, 2019

kresan requested review from fuhlig1 and MohammadAlTurany April 25, 2019 11:34

kresan force-pushed the feature_sensitive_volumes branch from 10f4ab5 to b7cd258 Compare April 26, 2019 08:41

FairRootGroup deleted a comment Apr 26, 2019

kresan force-pushed the feature_sensitive_volumes branch from b7cd258 to 3cd6899 Compare April 26, 2019 08:51

kresan requested a review from karabowi April 26, 2019 09:05

FairRootGroup deleted a comment Apr 26, 2019

dennisklein added this to the v18.4 milestone Jul 4, 2019

dennisklein removed this from the v18.4 milestone Apr 6, 2020

dennisklein added this to the V18.6 milestone Apr 29, 2020

kresan force-pushed the feature_sensitive_volumes branch from 3cd6899 to c6ffc2e Compare February 9, 2021 08:02

kresan removed the in progress label Feb 9, 2021

kresan force-pushed the feature_sensitive_volumes branch from 1a157c0 to bd19c48 Compare February 9, 2021 08:40

kresan added the in progress label Feb 9, 2021

dennisklein modified the milestones: v19.0, v19.2 Mar 8, 2021

dennisklein marked this pull request as draft March 8, 2021 16:17

karabowi force-pushed the feature_sensitive_volumes branch 2 times, most recently from 51bffcc to 011da54 Compare May 6, 2022 11:52

kresan force-pushed the feature_sensitive_volumes branch from ad6983f to 011da54 Compare June 8, 2022 07:09

karabowi force-pushed the feature_sensitive_volumes branch 2 times, most recently from a96f67f to 2259768 Compare June 8, 2022 08:02

karabowi force-pushed the feature_sensitive_volumes branch from 2259768 to 2ab950f Compare July 7, 2022 09:34

kresan and others added 6 commits January 26, 2023 12:23

Add support for TVirtualMCSensitiveDetector.

d814770

Fix Pixel detector

0197d72

Remove the double function definition.

Apply format

9af52a8

Fix the construction of cloned sensitive volumes

6196ba0

FairRoot allows for construction of clones volumes using the volume_name#{clone_number} scheme. Changing FairMCApplication::ConstructSensitiveDetectors() to support this.

Override two functions in FairMCApplication

3938cbd

karabowi force-pushed the feature_sensitive_volumes branch from 96b1b31 to 0612207 Compare January 30, 2023 14:26

karabowi force-pushed the feature_sensitive_volumes branch from 0612207 to b64b2fd Compare January 30, 2023 14:48

ChristianTackeGSI reviewed Jan 30, 2023

View reviewed changes

karabowi force-pushed the feature_sensitive_volumes branch from b64b2fd to e3810cc Compare January 31, 2023 09:44

karabowi added 2 commits February 1, 2023 10:20

TVirtualMCSensitiveDetector: example/Tutorial1 fixup

e34b574

Removed the IsSensitive() function. Increase the number of events to run in the MT mode to 20.

karabowi force-pushed the feature_sensitive_volumes branch from e3810cc to e34b574 Compare February 1, 2023 09:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for TVirtualMCSensitiveDetector. #889

Add support for TVirtualMCSensitiveDetector. #889

kresan commented Apr 25, 2019

sawenzel commented Apr 29, 2019

kresan commented May 6, 2019

kresan commented Feb 9, 2021

karabowi commented Feb 9, 2021

kresan commented Feb 9, 2021

karabowi commented Feb 9, 2021

MohammadAlTurany commented Feb 9, 2021 •

edited

Loading

fuhlig1 commented Feb 9, 2021

kresan commented Feb 9, 2021

MohammadAlTurany commented Feb 9, 2021

karabowi commented Feb 9, 2021

kresan commented Feb 10, 2021

fuhlig1 commented Jun 1, 2022

ChristianTackeGSI commented Jun 2, 2022 •

edited

Loading

fuhlig1 commented Jun 7, 2022 •

edited

Loading

fuhlig1 commented Jun 7, 2022

kresan commented Jun 8, 2022

fuhlig1 commented Jun 8, 2022

ChristianTackeGSI commented Jun 20, 2022

ChristianTackeGSI commented Jun 20, 2022

ChristianTackeGSI Jan 30, 2023

ChristianTackeGSI Jan 30, 2023

ChristianTackeGSI left a comment

		std::map<std::string, FairModule*>::iterator it;
		it = cloneVolumeMap.find(volName);

	std::map<std::string, FairModule*>::iterator it;
	it = cloneVolumeMap.find(volName);
	auto it = cloneVolumeMap.find(volName);

		Int_t id = fMC->CurrentVolID(copyNo);
		id = fMC->CurrentVolID(copyNo);

Add support for TVirtualMCSensitiveDetector. #889

Are you sure you want to change the base?

Add support for TVirtualMCSensitiveDetector. #889

Conversation

kresan commented Apr 25, 2019

sawenzel commented Apr 29, 2019

kresan commented May 6, 2019

kresan commented Feb 9, 2021

karabowi commented Feb 9, 2021

kresan commented Feb 9, 2021

karabowi commented Feb 9, 2021

MohammadAlTurany commented Feb 9, 2021 • edited Loading

fuhlig1 commented Feb 9, 2021

kresan commented Feb 9, 2021

MohammadAlTurany commented Feb 9, 2021

karabowi commented Feb 9, 2021

kresan commented Feb 10, 2021

fuhlig1 commented Jun 1, 2022

ChristianTackeGSI commented Jun 2, 2022 • edited Loading

fuhlig1 commented Jun 7, 2022 • edited Loading

fuhlig1 commented Jun 7, 2022

kresan commented Jun 8, 2022

fuhlig1 commented Jun 8, 2022

ChristianTackeGSI commented Jun 20, 2022

ChristianTackeGSI commented Jun 20, 2022

ChristianTackeGSI Jan 30, 2023

Choose a reason for hiding this comment

ChristianTackeGSI Jan 30, 2023

Choose a reason for hiding this comment

ChristianTackeGSI left a comment

Choose a reason for hiding this comment

MohammadAlTurany commented Feb 9, 2021 •

edited

Loading

ChristianTackeGSI commented Jun 2, 2022 •

edited

Loading

fuhlig1 commented Jun 7, 2022 •

edited

Loading